make 'opm serve' initialization asynchronous #977

joelanford · 2022-06-23T15:55:56Z

Signed-off-by: Joe Lanford [email protected]

Description of the change:
This PR modifies opm serve so that loading FBC is handled asynchronously in the background so that the grpc server can start immediately, thus enabling the health probes to return successfully faster.

All of the grpc endpoints that require the FBC to be loaded now block, waiting for the initialization to complete. Clients can now initiate a connection and the server will block the response until initialization completes. Clients that don't want to wait indefinitely can set timeouts on their requests.

Motivation for the change:
While we shipped a change in catalog-operator to make use of the startupProbe functionality, that fix will not solve problems caused by opm serve's long startup times on clusters without the startupProbe.

This fix solves that problem by ensuring the grpc server starts immediately (much like the sqlite-based grpc server).

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

openshift-ci · 2022-06-23T15:56:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelanford

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [joelanford]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2022-06-23T17:57:12Z

Codecov Report

Merging #977 (2c6e79b) into master (8250533) will increase coverage by 2.67%.
The diff coverage is 85.45%.

❗ Current head 2c6e79b differs from pull request most recent head 304c402. Consider uploading reports for the commit 304c402 to get more accurate results

@@            Coverage Diff             @@
##           master     #977      +/-   ##
==========================================
+ Coverage   52.60%   55.27%   +2.67%     
==========================================
  Files         104      130      +26     
  Lines        9422    10826    +1404     
==========================================
+ Hits         4956     5984    +1028     
- Misses       3536     3763     +227     
- Partials      930     1079     +149

Impacted Files	Coverage Δ
pkg/registry/query.go	`67.50% <85.45%> (+6.56%)`	⬆️
pkg/image/execregistry/registry.go	`54.54% <0.00%> (ø)`
pkg/lib/unstructured/unstructured.go	`60.71% <0.00%> (ø)`
pkg/api/grpc_health_v1/health_grpc.pb.go	`0.00% <0.00%> (ø)`
...inertools/containertoolsfakes/fake_label_reader.go	`0.00% <0.00%> (ø)`
pkg/sqlite/sqlitefakes/fake_rowscanner.go	`47.78% <0.00%> (ø)`
pkg/lib/log/writerhook.go	`0.00% <0.00%> (ø)`
pkg/lib/config/validate.go	`0.00% <0.00%> (ø)`
...s/containertoolsfakes/fake_dockerfile_generator.go	`0.00% <0.00%> (ø)`
pkg/image/containerdregistry/resolver.go	`57.53% <0.00%> (ø)`
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8250533...304c402. Read the comment docs.

joelanford · 2022-06-23T18:16:53Z

https://github.com/operator-framework/operator-registry/runs/7029116066?check_suite_focus=true#step:4:80

I think this is saying that var _ GRPCQuery = Querier{} would fail to compile. And I think this PR "breaks" that by making all of those Querier methods use pointer receivers. I don't think we ever intentionally tried to ensure Querier implemented GRPCQuery

However, we did intend for *Querier to implement GRPCQuery and that is still the case:

https://github.com/joelanford/operator-registry/blob/0a337c5535ae605d4b3fd4c86f9592d0180ce0ec/pkg/registry/query.go#L45

…t of grpc server Signed-off-by: Joe Lanford <[email protected]>

Signed-off-by: Joe Lanford <[email protected]>

joelanford · 2022-07-07T13:44:59Z

Makefile

 .PHONY: unit
 unit:
-	$(GO) test -coverprofile=coverage.out $(SPECIFIC_UNIT_TEST) $(TAGS) $(TEST_RACE) -count=1 ./pkg/... ./alpha/...
+	$(GO) test -coverprofile=coverage.out -coverpkg=./... $(SPECIFIC_UNIT_TEST) $(TAGS) $(TEST_RACE) -count=1 ./pkg/... ./alpha/...


This change means we now consider cross-package coverage. (i.e. the tests in pkg/server/server_test.go can contribute to coverage in pkg/registry/query.go)

This very likely means there's an artificial increase in the repo-wide coverage, but I did my best to add specific unit tests related to this PR in pkg/registry/query_test.go to make sure the new code paths are relatively well-covered.

dinhxuanvu

Looking good. Just one question.

dinhxuanvu · 2022-07-07T14:51:16Z

pkg/registry/query.go


-func (q Querier) ListBundles(ctx context.Context) ([]*api.Bundle, error) {
+func (q *Querier) ListBundles(ctx context.Context) ([]*api.Bundle, error) {
+	if err := q.Wait(ctx); err != nil {


At the moment, gRPC client on OLM side doesn't have a deadline setting so the rRPC calls can wait here for a long time. I wonder if we should set a deadline now so that if it goes for too long, it will fail out with DEADLINE_EXCEED error. Or perhaps "waiting until ready regardless how long it is" is a desire behavior? I would love this to be tested in a real cluster to see if things work well.

Wait has three cases when it returns:

The passed in context was canceled (not sure of all the ways that context is propagated from the GRPC server that calls this function, one way is if the server is shutdown). Perhaps this could be the avenue to set a timeout on a per request basis?

There was an error loading the FBC.

The FBC was successfully loaded.

So if Wait blocks, it means we're still churning through the initialization and haven't encountered any issues yet.

I'll take a look at the request deadline possibilities.

joelanford · 2022-07-07T16:35:02Z

/hold

There are some implications on OLM that we need to consider:

With the current request-blocking behavior, that might cause the OLM cache controller to block anytime it makes a request against a catalog source that is initializing. Blocking could manifest as higher latency reconciliations.
If we add a server-enforced timeout, the OLM cache controller could get deadline errors during catalog startup and cause unexpected behavior (e.g. failed resolutions, incorrect retry logic)

Signed-off-by: Joe Lanford <[email protected]>

openshift-ci · 2022-07-07T22:16:12Z

@joelanford: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jdockter · 2022-07-24T21:42:36Z

@joelanford is this issue on permanent hold or do you see this getting resolved as is?

joelanford · 2022-07-25T12:46:50Z

@jdockter I don't think it's permanently blocked. We just need to prioritize checking the implications of this change on clients of the GRPC server (catalog-operator and packageserver).

joelanford · 2022-08-24T13:12:18Z

Closing in favor of #1005

openshift-ci bot requested review from anik120 and exdx June 23, 2022 15:56

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2022

joelanford changed the title ~~make 'opm serve' initialization asynchronous~~ wip: make 'opm serve' initialization asynchronous Jun 23, 2022

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2022

joelanford force-pushed the opm-serve-async-startup branch 5 times, most recently from da0db50 to 0a337c5 Compare June 23, 2022 17:52

joelanford force-pushed the opm-serve-async-startup branch from 0a337c5 to d101180 Compare June 23, 2022 19:32

make 'opm serve' initialization asynchronous to enable immediate star…

44abc49

…t of grpc server Signed-off-by: Joe Lanford <[email protected]>

joelanford force-pushed the opm-serve-async-startup branch 2 times, most recently from f860fd8 to f658013 Compare July 6, 2022 15:27

joelanford changed the title ~~wip: make 'opm serve' initialization asynchronous~~ make 'opm serve' initialization asynchronous Jul 6, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2022

joelanford force-pushed the opm-serve-async-startup branch from f658013 to 7db066c Compare July 7, 2022 12:23

add tests for async fbc grpc server

eb0d664

Signed-off-by: Joe Lanford <[email protected]>

joelanford force-pushed the opm-serve-async-startup branch from 7db066c to eb0d664 Compare July 7, 2022 13:29

joelanford commented Jul 7, 2022

View reviewed changes

dinhxuanvu removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2022

dinhxuanvu reviewed Jul 7, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2022

opm serve: add --server-timeout flag with a default of 30s per request

304c402

Signed-off-by: Joe Lanford <[email protected]>

openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 7, 2022

alfieyfc mentioned this pull request Aug 16, 2022

fix(grpc): Add startupProbe to check for grpc health readiness operator-framework/operator-lifecycle-manager#2791

Merged

11 tasks

joelanford closed this Aug 24, 2022

joelanford deleted the opm-serve-async-startup branch August 24, 2022 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make 'opm serve' initialization asynchronous #977

make 'opm serve' initialization asynchronous #977

Uh oh!

joelanford commented Jun 23, 2022 •

edited

Loading

Uh oh!

openshift-ci bot commented Jun 23, 2022

Uh oh!

codecov bot commented Jun 23, 2022 •

edited

Loading

Uh oh!

joelanford commented Jun 23, 2022

Uh oh!

joelanford Jul 7, 2022

Uh oh!

dinhxuanvu left a comment

Uh oh!

dinhxuanvu Jul 7, 2022

Uh oh!

joelanford Jul 7, 2022

Uh oh!

joelanford commented Jul 7, 2022

Uh oh!

openshift-ci bot commented Jul 7, 2022

Uh oh!

jdockter commented Jul 24, 2022

Uh oh!

joelanford commented Jul 25, 2022 •

edited

Loading

Uh oh!

joelanford commented Aug 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

make 'opm serve' initialization asynchronous #977

make 'opm serve' initialization asynchronous #977

Uh oh!

Conversation

joelanford commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jun 23, 2022

Uh oh!

codecov bot commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joelanford commented Jun 23, 2022

Uh oh!

joelanford Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

dinhxuanvu left a comment

Choose a reason for hiding this comment

Uh oh!

dinhxuanvu Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

joelanford Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

joelanford commented Jul 7, 2022

Uh oh!

openshift-ci bot commented Jul 7, 2022

Uh oh!

jdockter commented Jul 24, 2022

Uh oh!

joelanford commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelanford commented Aug 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joelanford commented Jun 23, 2022 •

edited

Loading

codecov bot commented Jun 23, 2022 •

edited

Loading

joelanford commented Jul 25, 2022 •

edited

Loading